Review: PR #11871 — Convert bucket_key/requested_bucket_key to real foreign keys

Note: 11868 is the issue number; the PR for this branch is #11871. Reviewed that.
82 files, +2530/−852. Reviewed at head bd3b32ee0b.

Overview

Replaces the free-text bucket_key/requested_bucket_key string columns on signups,
signup_requests, signup_ranked_choices, and signup_changes with bucket_id/requested_bucket_id
FKs to registration_policy_buckets, backfills them by key, and drops the old columns.
signup_changes gets ON DELETE SET NULL plus bucket_name/requested_bucket_name snapshot columns
so audit rows survive bucket deletion. GraphQL keeps bucketKey/requestedBucketKey working but
deprecated and adds object/id fields, so the frontend needs no flag day.

The overall design is sound, and several decisions are well-argued in the comments: keeping
SignupBucketFinder and signup_definitely_occupies_slot_in_bucket? key-based for detached candidate
policies, snapshotting names on the audit table, and JSON-encoding SignupMoveResult#id now that names
are free text. The reordering of EventChangeRegistrationPolicyService around the destroy is the right
shape.

That said, there are four correctness issues and a set of N+1s introduced by turning column reads into
association reads.

Correctness

1. Policy change now fails when a removed bucket maps to "no preference" and the new policy has no flex bucket (regression)

apply_bucket_key_mappings moved from before the simulation to after it, replaced during simulation
by pending_requested_key_overrides. But that hash records only truthy mappings:

# app/services/event_change_registration_policy_service.rb:216
hash[signup.id] = to_key if to_key

So a to_key: nil mapping ("clear the preference") produces no override, and simulate_signup
falls back to signup.requested_bucket_key — the removed bucket's key.
SignupBucketFinder#prioritized_buckets_with_requested_bucket then resolves requested_bucket to nil
against the new policy and returns just [anything_bucket], instead of
prioritized_buckets_without_requested_bucket (all counted, limited buckets). With no flex bucket in
the new policy, find_bucket returns nil and the signup is reported immovable.

Verified with a probe test (removing unlimited, mapping to nil, new policy = dogs only, no flex):

• this branch: Signup for … would no longer fit, whole policy change fails
• main: succeeds, signup moved to dogs, requested_bucket_key cleared

The existing tests at test/services/event_change_registration_policy_service_test.rb:367-386 cover
to_key: nil, but new_registration_policy (line 9-17) always contains an anything bucket, which
masks it.

Fix: distinguish "no override" from "override to nil" — key the hash on mapping existence and read it
with key?:

hash[signup.id] = to_key if removed_bucket_ids.include?(signup.requested_bucket_id) &&
                            bucket_key_mappings.key?(removed_buckets_by_id[signup.requested_bucket_id]&.key)
# ...
requested_key = overrides.key?(signup.id) ? overrides[signup.id] : signup.requested_bucket_key

Please add a test with a flex-less target policy.

2. Admin "Bucket" filter on the run signups table silently returns zero rows

app/presenters/tables/signups_table_results_presenter.rb:77 changed column_filter :bucket_key →
column_filter :bucket_id, but RunSignupsTable.tsx's BucketFilter still submits keys and was not
updated by this PR:

// app/javascript/EventsApp/SignupAdmin/RunSignupsTable.tsx:92
value: bucket.key,

Signup.where(bucket_id: ["flex"]) compiles to WHERE "signups"."bucket_id" IN (NULL) (verified via
to_sql) — no error, no rows. The filter appears to work and returns nothing.

Either send bucket.id from BucketFilter, or keep the presenter resolving keys → ids in
apply_filter.

Relatedly, sql_order changed from bucket_key to bucket_id, so the Bucket column now sorts by
bucket row id (creation order) rather than key. Probably an improvement, but it's a silent user-visible
change worth a line in the PR body.

3. ForceConfirmSignup can 500

Both arguments became optional with no guard:

# app/graphql/mutations/force_confirm_signup.rb
argument :bucket_id, ID, required: false
argument :bucket_key, String, required: false, deprecation_reason: "Use bucketId instead"
# ...
signup.update!(state: "confirmed", bucket_id: bucket.id, ...)  # NoMethodError if bucket is nil

bucket_key was required: true before, so omitting both is newly schema-legal and yields a
NoMethodError on nil rather than a validation error. UpdateSignupBucket got a proper
GraphQL::ExecutionError guard — mirror it here. (Note the existing frontend sends
bucketKey: … ?? '', which already reaches the nil-bucket path.)

Same shape in UpdateSignupBucket#resolve: signup.run.bucket_full?(bucket_id) runs before the
update, and bucket_with_id returns nil for an id outside the run's policy → nil.full?. The lookup
being policy-scoped does correctly prevent cross-convention writes, so this is a 500-vs-error-message
issue, not a security one.

4. Missed conversion in notifier_preview_factory.rb

# app/notifiers/notifier_preview_factory.rb:115
SignupMoveResult.new(convention.signups.first.id, "confirmed", "flex", "waitlisted", nil)

Still passes a bucket key in the bucket_id position. The :prev_bucket_key → :prev_bucket_id
case just above was updated; this one wasn't. Notifier previews for signup-move templates will render
an empty bucket name.

Performance: N+1s introduced

Signup#bucket_key/#requested_bucket_key went from column reads to association reads, and nothing
preloads :bucket anywhere in app/. Measured, not inferred:

SignupBucketFinder construction: 10 signups → 10 queries against registration_policy_buckets.
FakeSignup.from_signup reads both bucket_key and requested_bucket_key for every other_signup.
EventSignupService#bucket_finder passes run.signups.to_a, so every signup attempt on a run with N
signups now issues N–2N extra queries. This is the signup hot path. Same in
EventChangeRegistrationPolicyService#simulate_signups (plus
signups.partition { … !signup.bucket_key }).

RegistrationPolicyBucket#signup_definitely_occupies_slot_in_bucket? compares
signup.bucket_key == key, so available_slots/full?/has_available_slots? — and therefore
Run#full?, Run#available_slots_by_bucket_id, the schedule grid, and the capacity graphs — now issue
a query per signup where they read a column before. Cheapest fix that preserves the detached-policy
contract: compare signup.bucket_id == id when id is present, falling back to key only for
FakeSignup/unpersisted buckets.

GraphQL: Types::SignupType adds :requested_bucket to association_loaders but resolves
bucket via object.bucket — a query per signup on every signup table/list.
Types::SignupChangeType has the same gap (object.bucket in both bucket and exposed_bucket?,
:bucket absent from its association_loaders). Also
EventVacancyFillService#signup_can_fill_bucket_vacancy? (signup.bucket.nil?) and
Signup#log_signup_change! (bucket&.name, requested_bucket&.name).

None of these are hard to fix, but they're spread across hot paths, and there are no query-count
assertions in the suite to catch them.

Deploy risks

In-flight jobs will fail during the deploy window. Signups::WithdrawalNotifier and
WithdrawConfirmationNotifier renamed a required kwarg (prev_bucket_key: → prev_bucket_id:), and
Notifier#deliver_later enqueues initializer_options as job args. Jobs enqueued by the old code
(5-second delay, plus any retry backlog) will raise
ArgumentError: unknown keyword :prev_bucket_key; missing keyword :prev_bucket_id.
SignupMoveResultSerializer#deserialize has the same shape — it reads hash[:bucket_id], so old
payloads silently lose bucket info. Suggest accepting both spellings for one release, or draining the
queue before cutover.

Both migrations ship in one release and drop the string columns immediately. Any old container
still serving while migrations run will 500 on signups.bucket_key. The issue explicitly floated a
phased dual-write as the safer option; the PR body doesn't say why it was dropped. If the deploy is
single-container stop-then-start this is fine — worth stating explicitly either way.

Backfill is unbatched. UPDATE signups SET bucket_id = … FROM runs JOIN … rewrites every row in
signups and signup_changes in one statement inside the migration transaction, and add_reference
builds its indexes non-concurrently. There's no strong_migrations in the Gemfile to flag this. For
production-sized signup_changes, consider disable_ddl_transaction! + algorithm: :concurrently and
a batched backfill.

Smaller notes

• clear_references_to_removed_buckets nulls requested_bucket_id for all signups on the event's
  runs, but apply_requested_bucket_mappings_for_signups(all_signups) only re-resolves non-withdrawn
  ones (all_signups excludes withdrawn). Withdrawn signups' preferences are now silently dropped on
  any bucket removal, even with a mapping supplied. Previously they were left stale. Likely harmless —
  but undocumented.
• SignupMoveResult#id Base64+JSON is the right call given free-text names, but
  SignupMoveResult.find now raises on any old-format id. No in-repo callers of the GlobalID path, so
  low risk; worth confirming none are persisted anywhere.
• ForceConfirmSignup uses signup.run.registration_policy in one branch and
  signup.run.event.registration_policy in the other — equivalent, but pick one.
• EventSignupService#requested_bucket memoizes after the early return unless, so a nil result
  re-queries on each call. Cosmetic.
• The synthetic id: 'signups' in useEventForm.tsx is safe server-side (build_from_hash strips
  id via IGNORED_HASH_KEYS), but it's a string in an ID! field — a comment would help the next
  reader.

Test coverage gaps

• No case for to_key: nil against a policy without a flex bucket (finding 1 — every existing case
  has one).
• No coverage of the signups-table bucket filter (finding 2).
• No coverage of ForceConfirmSignup with neither argument (finding 3).
• No query-count assertions anywhere in the signup path, which is why the N+1s slipped through a green
  suite.

Verdict

The data model change and the compatibility strategy are right, and the tricky ordering in
EventChangeRegistrationPolicyService is mostly handled thoughtfully. I'd hold merge on findings 1–3
(all small, localized fixes plus tests), decide explicitly on the notifier-kwarg compatibility shim
before deploy, and either fix the N+1s here or file them as an immediate follow-up — finding 6 in
particular touches the schedule grid, which is one of the heaviest pages in the app.

Review written by Claude

Fixes for review findings

Addressed all four correctness bugs and the N+1s from the review, plus the deploy-risk and smaller-notes items, in 6dcb4dfef4.

Correctness

1. Policy-change nil-mapping regression — EventChangeRegistrationPolicyService::SignupSimulator now distinguishes "no override" from "override to nil" (via a NO_OVERRIDE sentinel and pending_requested_key_overrides keying on mapping presence), so clearing a preference on a removed bucket no longer falls back to the removed bucket's key. Added a test with a flex-less target policy (event_change_registration_policy_service_test.rb) that reproduces the original bug and fails without the fix.
2. Admin bucket filter — RunSignupsTable.tsx's BucketFilter now submits bucket.id instead of bucket.key (added id to the buckets{} selection everywhere in queries.graphql, not just the one query). Added test/presenters/tables/signups_table_results_presenter_test.rb covering the filter. Also, per an additional request, changed the Bucket column's sort from bucket_id to case-insensitive bucket name (left-joined so unassigned signups still appear).
3. ForceConfirmSignup/UpdateSignupBucket 500s — both now raise GraphQL::ExecutionError for a missing/invalid bucket instead of NoMethodError on nil; also unified ForceConfirmSignup's registration_policy accessor. Covered by new test/graphql/mutations/force_confirm_signup_test.rb and update_signup_bucket_test.rb.
4. notifier_preview_factory.rb — stopped passing a stale bucket key ("flex") where a bucket id was expected; now uses the actual signup's bucket_id. Covered by test/notifiers/notifier_preview_factory_test.rb.

N+1s

• Preloaded :bucket/:requested_bucket before building SignupBucketFinder/FakeSignup in EventSignupService, EventChangeRegistrationPolicyService, and ExecuteRankedChoiceSignupService.
• RegistrationPolicyBucket#signup_definitely_occupies_slot_in_bucket? now compares bucket_id directly for persisted signups against a persisted bucket, falling back to the key comparison only for FakeSignup/detached buckets (extracted into occupies_bucket_as_signup? to keep complexity in check).
• Added :bucket to the dataloader-batched association loaders in Types::SignupType and Types::SignupChangeType (both previously called object.bucket directly, bypassing batching).
• Preloaded buckets in EventVacancyFillService#all_signups and via SignupMoveResult#signup.

Added query-count regression tests (test/models/run_test.rb, test/services/event_signup_service_test.rb) that assert bucket-table query counts stay constant regardless of signup count — confirmed 11→2 queries and 22→4 queries before/after the fix.

Deploy risk

Split the single migration so this release only adds/backfills bucket_id/requested_bucket_id, keeping the legacy bucket_key/requested_bucket_key columns in place. Filed #11872 with the column-drop migration to run in a later release once the backfill is verified in production.

Smaller notes

Documented the withdrawn-signup preference-clearing behavior change in clear_references_to_removed_buckets, and added a comment on the synthetic bucket id in useEventForm.tsx.

Incidental cleanup

Fixing the bucket filter surfaced a shadowed Lint/MissingSuper entry in .rubocop_todo.yml (a duplicate YAML key silently dropped two exclusions) and missing id selections on several buckets{} queries flagged by @graphql-eslint/require-selections once the file was touched — fixed both. Also downgraded @graphql-eslint/no-deprecated to a warning for .graphql files, since bucket_key/requested_bucket_key are deliberately still queried in several places for frontend backward compatibility during this migration.

All 353 relevant Ruby tests pass, tsc --noEmit is clean, and rubocop/eslint report no errors on the changed files.

Comment written by Claude

Manual QA plan

This PR converts bucket_key/requested_bucket_key from free-text strings to real foreign keys (bucket_id/requested_bucket_id), and this session's follow-up fixed 4 correctness bugs, several N+1s, and changed the admin Bucket column to sort by name. The plan below is organized so the highest-risk areas (registration policy edits, the admin bucket filter, force-confirm) come first, then broader regression coverage of everything else that touches buckets.

Suggested test convention: an event with a limited counted bucket (e.g. "Dogs"), an unlimited/flex bucket (e.g. "Anything"), and a run with a handful of signups split across them plus a couple of waitlisted signups.

1. Registration policy editing — bucket removal + remapping (highest risk)

This is the scenario behind the policy-change simulation bug: clearing a signup's bucket preference on a removed bucket, when the new policy has no flex bucket to fall back to.

• On an event's edit page, in the Registration policy card, sign up an attendee with a requested bucket preference for a bucket you're about to remove.
• Remove that bucket from the policy so the only remaining buckets are limited/counted (no "anything"/flex bucket), and save.
• In the Remap bucket assignments modal that appears, set "Move attendees to" to No preference for the removed bucket, then Apply and save event.
• Confirm the save succeeds (previously this could fail with "Signup for <name> would no longer fit" even though a normal-capacity bucket was available) and the attendee's signup lands in a real bucket, not left stranded.
• Repeat, but this time map the removed bucket to a specific surviving bucket — confirm the signup moves there as expected.
• Repeat once more with a policy that does keep a flex/anything bucket — confirm existing "moves to flex" behavior still works normally.
• Try removing a bucket that still has pending signups/requests without providing a mapping — confirm you still get a clear "no mapping provided" error (not a crash).

2. Admin Signups table — Bucket filter and sort

Route: Event → Run → Signups tab.

• Use the Bucket column filter to select a specific bucket. Confirm it returns the signups actually in that bucket (previously this silently returned zero rows due to a key/id mismatch).
• Select multiple buckets in the filter and confirm rows from all selected buckets show up.
• Click the Bucket column header to sort ascending, then descending. Confirm rows are ordered alphabetically by bucket name (case-insensitively — e.g. "anything" and "Dogs" sort by name, not by creation order/internal id), and that signups with no bucket (waitlisted) sort predictably to one end.
• Export the table via Export CSV and confirm the Bucket column in the CSV looks correct.

3. Force-confirm and change-bucket (Edit Signup page)

Route: Signups table → click a row → Edit signup.

• On a waitlisted signup, click Force into run, pick a bucket, and confirm it succeeds and the signup becomes confirmed in that bucket.
• On a confirmed signup, click the change-bucket (pencil) icon next to Bucket, pick a different bucket, and confirm the move succeeds and frees up/fills the vacated bucket correctly (check another waitlisted signup gets pulled in if applicable).
• If reachable through the UI, try submitting Force Confirm / Change Bucket without a bucket selected — confirm you get a clear error message rather than a blank page or server error. (This was previously a 500 error under the hood; if the UI always forces a selection, it's fine to note that as "not reproducible via UI" rather than force it.)

4. Self-service signup (attendee-facing)

• Sign up for a run with a specific bucket preference — confirm you land in that bucket (or waitlist if full) and the schedule grid/status badge reflects it correctly.
• Sign up with no preference — confirm placement follows the usual "flex bucket" / most-available-counted-bucket logic.
• Sign up for a full bucket — confirm you're waitlisted, and that withdrawing another attendee from that bucket pulls you in automatically (vacancy fill).
• Withdraw a confirmed signup and confirm the vacancy-fill cascade behaves as before (waitlisted attendees move in, in priority order).

5. Ranked-choice / lottery conventions (if applicable to your test convention)

• On My Signup Queue, add and reorder a few ranked-choice preferences (drag and drop).
• Trigger or wait for a signup round to execute (Signup Rounds admin, or the manual "rerun" action), then check Round Results — confirm decisions (confirmed/waitlisted/skipped) and bucket assignments look correct, and the results table/CSV export works.

6. Signup change history

Route: Signups tab → Change history tab.

• Confirm the Bucket column shows correct bucket names for past changes, including for a signup whose bucket assignment involved a bucket that has since been deleted (name should still display via the historical snapshot, not blank).
• Export the change history CSV and spot-check the Bucket column.

7. Notification preview

Route: Notifications admin (/admin_notifications).

• Find the notification for signups moving due to a bucket change (or "user signup moved") and click Preview. Confirm the rendered preview shows a real bucket name rather than a blank/missing one.
• Preview a couple of other signup-related notifications (confirmation, waitlist) to confirm nothing else regressed.

8. Freeze bucket assignments

Route: Signups tab → Freeze bucket assignments button.

• Run it on a run with some flex-bucket occupants who have a requested bucket, and confirm they get moved into their requested bucket and the flex bucket shrinks accordingly, per the confirmation dialog's description.

9. GraphQL API / backward compatibility

• In GraphiQL (or the API docs), confirm bucketKey/requestedBucketKey fields on Signup are marked deprecated but still return correct values, and the new bucket/requestedBucket object fields return the full bucket (name, id, key, etc.).
• If you have an API client or integration still using the deprecated key-based fields, do a quick smoke test that it still works unchanged.

10. General regression pass

• Import convention data (CSV import) that includes signups with bucket assignments — confirm buckets resolve correctly.
• Browse the schedule grid / capacity views for a convention with several runs and confirm bucket capacity displays (full/available) are accurate — this is also a good place to eyeball page load time, since several N+1 query bugs in this area were fixed.
• General smoke test of moderated signup requests (accept/reject with a specific bucket) if your convention uses moderated signups.

Notes for whoever picks this up

• The four correctness bugs and N+1s each have an automated regression test now, but automated tests can't fully substitute for actually clicking through the flows above, especially the registration-policy remapping modal (§1) and the admin bucket filter/sort (§2), which are the two areas most likely to have UI-level surprises the tests didn't cover.
• If anything in here reproduces a bug, please note which checkbox and paste any error message/screenshot in a reply.

QA plan written by Claude

Bug found in QA Section 1, fixed in ffc224b3f4

While testing the registration-policy bucket removal + remapping flow, saving with a bucket mapped to No preference (the default dropdown selection) failed with Bucket key "signups" was removed but no mapping was provided, even though the modal showed the mapping as selected.

Root cause: BucketKeyRemappingModal is mounted once, up front, with removedBuckets: [], before the admin ever clicks Save. removedBuckets only becomes non-empty later, as a prop update on that already-mounted component. Its mappings state was seeded via a useState lazy initializer, which only ever ran against that initial empty array — so a removed bucket's default "No preference" selection (already shown as selected in the dropdown) never got an entry in state unless the admin explicitly changed the dropdown. Since handleConfirm builds the submitted mappings from Object.entries(mappings) rather than from removedBuckets, accepting the default silently sent an empty list, and the (correct) server-side check rejected it.

This is a pre-existing bug in BucketKeyRemappingModal.tsx/useBucketKeyRemapping.tsx (introduced in #11229, well before #11868/#11871) — not something the bucket FK conversion caused. It just happened to block QA Section 1 of the plan above. Fixed by re-seeding mappings whenever removedBuckets changes (React's "adjusting state during render" pattern), with a regression test in test/javascript/EventAdmin/BucketKeyRemappingModal.test.tsx.

Section 1 of the QA plan should be unblocked now — worth re-testing the "map to No preference by leaving the default" case specifically, since that's exactly the path this bug broke.

Comment written by Claude

Second bug found in QA Section 1, fixed in bfc02b2df5

In an event whose registration policy has "no preference" signups disabled (prevent_no_preference_signups), the remap modal still let you map a removed bucket to No preference. That would leave affected signups/requests/ranked choices with a null requested_bucket_id — a state new signups aren't allowed to be created in under that policy, so existing records could drift into an inconsistent state.

Fixed on both sides:

• Frontend: BucketKeyRemappingModal hides the "No preference" option and disables Apply until every removed bucket has an explicit destination bucket chosen, when the new policy disallows no-preference signups.
• Backend: EventChangeRegistrationPolicyService now rejects a to_key: nil mapping outright when the new policy's prevent_no_preference_signups is set — defense in depth for anything that bypasses the modal (direct API use, future UI paths, etc.).

Both have regression tests (test/javascript/EventAdmin/BucketKeyRemappingModal.test.tsx, test/services/event_change_registration_policy_service_test.rb).

Comment written by Claude

Code Coverage Report: Only Changed Files listed

Minimum allowed coverage is 0%, this run produced 55.88%